Skip to content

Conversation

@barnabasdomozi
Copy link
Collaborator

No description provided.

Copy link
Contributor

@bruntib bruntib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests would also be useful.



def __process_file(file_path: str) -> Tuple[str, List[str]]:
with open(file_path, 'rb') as fp:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with open(file_path, 'rb') as fp:
with open(file_path, 'rb', encoding='utf-8', errors='ignore') as fp:

Conventionally we're using utf-8 encoding for every open(). We should get rid of this in the future by setting encoding globally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are opening the file in binary mode, where encoding and errors options shouldn't be specified, see docs: https://docs.python.org/3/library/functions.html#open

For XML parsing, the plistparser uses lxml, see (tools/report-converter/codechecker_report_converter/report/parser/plist.py, line 104). It accepts the data in binary form, thus we are reading the file in binary mode.


def __create_tables(self):
if not self.__table_exists("plist_lookup"):
self.__cur.execute("CREATE TABLE plist_lookup(plist, source)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't column types be indicated here? The default type is BLOB (https://www.sqlite.org/datatype3.html: 3.1)

Comment on lines 96 to 100
plist_files = list(filter(lambda f: f.endswith(
plistparser.EXTENSION), os.listdir(report_dir)))
plist_files = list(map(lambda f: os.path.abspath(
os.path.join(report_dir, f)), plist_files))
plist_files = list(filter(lambda f: f not in indexed_files, plist_files))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List generation is not necessary. I think, the generator objects could be nested into each other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in commit 6b7cce9

@barnabasdomozi
Copy link
Collaborator Author

Some tests would also be useful.

Thanks for the review! I will add tests in a separate commit.

@dkrupp dkrupp added this to the release 6.27.0 milestone Sep 10, 2025
@barnabasdomozi
Copy link
Collaborator Author

@bruntib @Discookie
I added unit tests for CacheDB and functional tests for the reindex command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants